Skip remote content upload when the remote cache is read-only - #178
Skip remote content upload when the remote cache is read-only#178Pavlo Tubolets (tubolets) wants to merge 3 commits into
Conversation
CasCacheClient holds two sessions: the raw _remoteCacheSession and the _twoLevelCacheSession built from TwoLevelCacheConfiguration. When adding a node, content was uploaded directly through _remoteCacheSession without consulting RemoteCacheIsReadOnly, while the content hash list that would reference that content is published through _twoLevelCacheSession, which does honor it. The result is that a read-only client uploads content that can never be retrieved: no content hash list ever points at it, so no subsequent build can hit on it. The upload only costs egress, storage and build time, and leaves unreferenced blobs behind for lifecycle policies to collect. Guard the upload block on the read-only flag, which also elides the remote pin calls that were only made to decide what to upload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| // this content is not published either (TwoLevelCacheSession honors RemoteCacheIsReadOnly below), so any | ||
| // content uploaded here could never be retrieved by a subsequent build. Uploading it would only cost | ||
| // egress, storage and time. | ||
| if (_remoteCacheSession != null && !_remoteCacheIsReadOnly) |
There was a problem hiding this comment.
Please add a regression test that verifies AddNodeAsync makes no PinAsync or PutStreamAsync calls on the remote session, while still writing the node to the local cache. The existing CasCacheClientTests only exercise GetAddNodeResult.
There was a problem hiding this comment.
Done — added in 9555de7.
There were no ICacheSession/ICache test doubles in Common.Tests, so I added RecordingCacheSession and RecordingCache under Mocks/. They count the calls made against them and throw on anything the tests do not exercise. The tests drive the public CacheClient.AddNodeInternalAsync, which reaches AddNodeAsync without needing outputs on disk.
Two tests:
AddNodeDoesNotUploadContentWhenRemoteCacheIsReadOnly— asserts the remote session sees noPinAsync,PutStreamAsyncorPutFileAsynccalls, and that the node metadata and content hash list are still written to the local cache.AddNodeUploadsContentWhenRemoteCacheIsWritable— asserts the remote session is still pinned and uploaded to when the remote is writable, so the guard is conditional rather than someone later making it unconditional.
The local session reports pins as succeeding (so nothing needs ingesting from disk) and the remote reports them as missing (so a writable remote would upload).
I confirmed the regression test fails with the fix reverted and the tests unchanged:
Assert.AreEqual failed. Expected:<0>. Actual:<1>. 'actual' expression: 'remoteSession.PinCallCount'.
The remote session must not be pinned when the remote cache is read-only.
while AddNodeUploadsContentWhenRemoteCacheIsWritable still passed.
There was a problem hiding this comment.
found a small bug, addressed in the next commit
David Federman (dfederm)
left a comment
David Federman (dfederm)
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Just 1 comment to add a regression test, but otherwise looks great!
Adds RecordingCacheSession / RecordingCache, minimal ICacheSession and ICache implementations that count the calls made against them, and two tests driving CasCacheClient.AddNodeInternalAsync: - AddNodeDoesNotUploadContentWhenRemoteCacheIsReadOnly asserts the remote session sees no PinAsync, PutStreamAsync or PutFileAsync calls, while the node metadata and content hash list are still written to the local cache. - AddNodeUploadsContentWhenRemoteCacheIsWritable asserts the remote session is still pinned and uploaded to when the remote cache is writable, so the guard is conditional rather than unconditional. The first test fails without the accompanying fix: Expected:<0>. Actual:<1>. 'actual' expression: 'remoteSession.PinCallCount'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@microsoft-github-policy-service agree company="WiseTech Global" |
2 participants
RemoteCacheIsReadOnlysuppresses content hash list publication but not content upload, so a read-only client uploads content that no later build can ever retrieve — nothing references it. It costs egress, storage and build time for no benefit.CasCacheClientholds both the raw_remoteCacheSessionand the_twoLevelCacheSessionbuilt fromTwoLevelCacheConfiguration.AddNodeAsyncuploads content through the raw session without consulting the flag, while the content hash list goes through the two-level session, which honours it.Changes
RemoteCacheIsReadOnlyfrom the passedTwoLevelCacheConfigurationand skip the remote upload block when it is set. This also elides the remotePinBulkAsynccalls, which were made only to decide what to upload.Validation
dotnet build MSBuildCache.sln -c Release: succeeded, 0 warnings, 0 errorsdotnet test MSBuildCache.sln -c Release --no-build: 547/550 passed. The 3net472failures are a pre-existingThe tools version "Current" is unrecognizedenvironment issue inMSBuildCachePluginBaseTests; an unmodified checkout fails the same 3.No unit test added: there is no mock
ICacheSessioninCommon.Teststo build anAddNodeAsyncharness on, andCasCacheClientTestscurrently only covers the staticGetAddNodeResulthelper. Happy to add a mock session and a regression test if you'd like it in this PR.Fixes: #177